fix: correct message id generation and base64 payload decoding - #697
owenpearson wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughGenerated IDs for REST annotations and messages now use URL-safe Base64. Base64 decoding now handles decoding and encoding errors without propagating them, and a test checks that encoded message data remains unchanged. ChangesIdempotent Publishing IDs
Base64 Decoding Errors
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change uses URL-safe Base64 for generated IDs and preserves payloads when decoding raises. No concrete current-head failure is established; external-service acceptance of the URL-safe alphabet remains unverified, so normal integration checks are appropriate. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Keeping an undecodable message available to consumers is intentional, but that message can also become the decoding base for later messages. This could disrupt delivery on channels using delta decoding; no broader access or privilege change was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the encoded stream, Comment |
bcb3afc to
40895a2
Compare
40895a2 to
30a7686
Compare
30a7686 to
420379a
Compare
420379a to
61d4507
Compare
61d4507 to
80151fe
Compare
80151fe to
05ff05f
Compare
05ff05f to
e0abe60
Compare
e0abe60 to
0c6db32
Compare
0c6db32 to
37dad70
Compare
37dad70 to
e47e873
Compare
…habet An id travels in a URL path, where the standard alphabet's `+` and `/` are not safe to carry. RSL1k1 asks only for "base64-encoding a sequence of at least 9 bytes" and names no alphabet, so both encodings conform. `test_idempotent_library_generated` decodes a generated id, and needs the matching decoder to do so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`base64.b64decode` raises on a payload that is not valid base64, and the exception escaped `Message.from_encoded` rather than being reported. RSL6b asks for the failure to be logged and the message delivered with the encodings that were not applied, which is what the missing-cipher and unsupported-encoding branches alongside it already do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e47e873 to
81ed7b1
Compare
PR 3 of 9 in the UTS REST unit stack. Base:
uts/rest-payload-fixes.Two small, independent fixes.
Message and annotation ids now use the URL-safe base64 alphabet. An id travels in a URL
path, where the standard alphabet's
+and/are not safe to carry. RSL1k1 asks only for"base64-encoding a sequence of at least 9 bytes" and names no alphabet, so both encodings
conform — but only one is safe in a path.
A message whose base64 payload cannot be decoded is now delivered rather than dropped.
base64.b64decoderaises on a payload that is not valid base64, and the exception escapedMessage.from_encoded. RSL6b asks for the failure to be logged and the message deliveredwith the encodings that were not applied, which is what the missing-cipher and
unsupported-encoding branches alongside it already do.
Review notes
The two commits touch disjoint files and can be reviewed independently. Worth noting that
the URL-safe change makes
RSL1k2/message-id-format-0andRSAN1c4/idempotent-id-generated-0pass, but those specifications assert
[A-Za-z0-9_-]+against afeatures.mdrequirementthat names no alphabet — so they would reject a conforming SDK's ids at random. That is
recorded as a specification fault, not fixed here.
Verification
80 passed(test/unit);ruff check ably/ test/clean.🤖 Generated with Claude Code
Summary by CodeRabbit